Separate state transition validation from nonState transition valdations #12120
Conversation
|
Jenkins results:
|
There was a problem hiding this comment.
@todor-ivanov as we have already found 2 issues with this development, I would like to ask you to run a representative validation for this fix. By representative, I can think of at least the following use cases:
- create and assign standard workflow
- create and assign ACDC workflow
- change priority for active workflow
- change site lists for active workflow
- change priority and site lists for active workflow
Once we cover all these use cases, then you could look into fixing the unit tests as well.
|
Can one of the admins verify this patch? |
|
@todor-ivanov I am trying to organize WM central services upgrade and I need to know how this validation is progressing and when you think you can finish it? If you think it can still take a few days, then we might actually revert the 2 changes that went in and give you the time you need to validate this. |
|
@amaltaro I am pretty sure I will not be able to finish this before mid next week. |
Add proper checks for allowed request properties changes && Stop reducing request_args only to RequestPriority for noStatusTransition actions Extend allowed arguments including stat_keys and RequestStatus && Call the relevant modifiers from _handleNoStatusUpdate Fix missing RequestName from request_args on mutipple calls of validate_request_update_args Add proper log messages for Sitelists changes Remove redundant validation calls && Update reqdb couch with single action && Move reduceReport to Utils. Typo Remove forgotten commented lines of code Review comments Source files pylint fixes Review comments
Unit tests Unit tests pylint fixes Unit tests - remove tests for reduceReport
|
hi @amaltaro: Coming back to the tests requested here: #12120 (review)
Here are two of the workflows I used for these tests: |
|
hi gain @amaltaro , and here are the results for an artificially created ACDC: This is the workflow: https://cmsweb-test1.cern.ch/reqmgr2/fetch?rid=tivanov_ACDC_TaskChain_LumiMask_multiRun_SiteListsTest_v6_241018_134936_9617
Which is kind of expected, since in the
|
|
I did yet another test. With and without the patch from this PR applied:
Which means we end up here: instead of here: So which means we may need to either repeat the actions from
What do you think? |
|
BTW, up until now we did not feel the difference for workflow parameter change with and without state update from @amaltaro any ideas? |
|
Ok @amaltaro , I tested a really nasty workaround, following the path for calling Here are the logs from:
There is a side effect, though, Besides the fact that we get into to spiral of calls within calls of methods which could be aligned sequentially and only skip the irrelevant ones.... but anyway. The side effect I am speaking is actually something I think I've seen in the past, which is - Once you update any of the request parameters in the web interface while the ACDC is in |
todor-ivanov
force-pushed
the
feature_SeteWhitelist_SupportChangeInReqmgr2_fix-12307
branch
from
0111d4d
to
62a6d43
Compare
|
@todor-ivanov given that this PR was started while we had the 3 pull requests merged into master/head, wouldn't it make this development more clean if we apply the changes in this PR on top of #12148? Or having a second look into the commits in this one, it looks like it has all the commits from #12148. If that is the case, should we close out #12148 to avoid any possible confusion? In addition, please make sure the initial PR description is up-to-date. |
|
hi @amaltaro that was my idea as well. I am pretty much in favor of closing #12148. While testing the fix about the broken I'll merge the two PR descriptions as well. |
|
And before I forget to say it once again, when validating this feature a couple of weeks ago, I noticed that we could update the site lists in ReqMgr2 Web UI as well. It looks like the original idea for If we want to repurpose that, I think it is fine. But I am not very comfortable with allowing users to change site lists in the Web UI. It's very much error-prone, and given the cost of this operation across the system, we need to keep it as low as we can. Said that, @todor-ivanov can you please check with the P&R team on the actual needs to have this feature in the Web UI as well? If they are happy having this feature only through REST API - programmatically - that would be the best IMO. |
|
Hi @amaltaro, if I am to revert this at this stage, it would require a significant refactoring of the whole idea behind the whole change. I am not even sure it would be possible, because this is how it is validated - this is my understanding of the code even at the current stage. WE were simply ignoring anything sent with the user's request and just setting up the priority field (all the rest we were setting to zero). I am not sure we are actually changing any behavior. To it seems we are all good. . And I actually do find it quite useful to have it exposed to the WEB UI. At the end adding a feature to be available through one interface and not through the another... would be yet one more |
|
The way the REST and Web UI are constructed is different, so there is no conditional statement involved in this story. I also agree that having it in the Web UI is useful. But my concern is on typos and mistakes by using the Web UI, which can perhaps lead to error. Right now, Web UI is only used for ACDC assignment, AFAIK. |
the SiteLists are drop down menus. |
|
And we got the P&R reply -The'd prefer to have the WEB UI interface as well |
|
Thank you Todor. That would have been my answer as well, from the user perspective. |
|
test this please |
|
@amaltaro @todor-ivanov I propose to keep going with the proposed solution, with claryfing to operators they will be accountable for any disruptions induced with improper use of this functionality. I have only question:
Btw, @todor-ivanov there are some failing checks for Jenkins |
|
hi @anpicci :
Yes. The site names are predefined and already populated in the list of possibilities:
I cannot make sense of these so far. @khurtado how should I read them? I see you have cancelled the tests, are those reliable at this stage? |
Fixes #12037
Status
READY
Description
SUPERSEDES: #12146 && #12148
With the current change we allow SiteLists related actions for statuses: staging, acquired, running-open in ReqMgr2
Before making a call to central services for changing any of the request parameters, an additional step is executed to
to check which are the allowed parameter modifications for the given status and if the so provided new values from the rest call actually differ from the workflow parameters already defined in central couchdb.
With the current change we no longer ignore all the rest of the request arguments provided with the REST call in the cases of a change of the Request priority. See: #8457 (comment)
The current change is also meant to address the issue with vanishing parameters during assignment of ACDC workflows as explained here: #12037 (comment). Even though, the issue would have manifested itself for regular workflows as well, if they were to experience any parameter change in a state transition from
assignment-approvedtoassigned. Currently this process is taken by Unified (and I believe no parameter change was happening during this, so automated, step) and the only manual intervention we perform at this state transition was for ACDC ... ,hence why we noticed the misbehavior with an ACDC workflow.With the current PR we suggest a separate logic in the validation function between
stateandnon-statetransition workflow updates.Is it backward compatible (if not, which system it affects?)
YES
Related PRs
This PR provides a cherry-pick of 3 pull requests that have been recently reverted. Changes have been originally provided by:
#12077
#12108
#12111
External dependencies / deployment changes
No